fix(people): protect governed mutation runtime types - #64
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughPeople 변이와 채용 수락 경계가 서브클래스를 거부하도록 변경되었습니다. UUID, 문자열, Decimal, 명령, 결과, 권한 결정, 시간값, PostgreSQL 행 형태에 정확한 타입 검사를 적용했습니다. 런타임 무결성 회귀 테스트와 변경 기록을 갱신했습니다. ChangesPeople 런타임 무결성
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to This change hardens People mutation and hire-acceptance inputs against executable subtype values while preserving supported built-in values and persistence behavior. No concrete current-head merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Exact-head repair/evidence update for the current branch state:
Fresh exact-current-head hosted evidence is terminal GREEN: People API Quality Live protected base remains |
|
Current exact-head follow-up after fresh review: A new review correctly identified that the three direct
Fresh exact-current-head hosted evidence is terminal GREEN:
Live protected base remains |
|
Fresh exact-head review of #272 authority has been repaired accordingly. It now distinguishes deterministic synthetic PostgreSQL mechanism/integration evidence from real/right-cleared commercial/scientific acceptance, and explicitly requires failure paths to release/rollback and join both writers before container teardown. This does not authorize a new production lock or reintroduce #271. Current Foundation |
|
Fresh exact-head evidence update for
No People source churn, no-op retrigger, Security shim, predecessor-evidence transfer, or gate bypass is warranted for the central Dependency Review incident. Keep Draft. |
|
Fresh exact-head gate sweep on |
|
Final fresh CodeQL materialization supersedes my earlier queued snapshot. Exact |
|
Fresh exact-head authority correction for
Keep Draft. Do not transfer predecessor evidence, no-op retrigger, self-approve or bypass. Separate DDD handoff: protected architecture assigns Position to |
|
Fresh current-head authority correction for unchanged
No source churn or no-op retrigger is justified by these terminal central-owner failures. #272 mechanism evidence stays GREEN; the separate real/right-cleared commercial/scientific tier remains unresolved and must not be inferred from synthetic integration fixtures. |
Scope
Canonical Orgmetra People mutation runtime/contract-integrity lane. HR mutation truth stays in Orgmetra; this PR does not copy mutable shared-kernel, authorization, workflow-owner, People-read, or downstream Assignment source.
Protected base is
develop@eb9757f8649aaad026a9865508d9aad50c1a7a4f. Current exact head is818950960c4fe0a4e7b026bd5499fe9cca1002be; the PR remains open · Draft · mechanically mergeable. Predecessor checks/reviews are causal evidence only and do not authorize this head.Retained repair chain
#215–#225 retain hire/generic PostgreSQL trust-boundary hardening. #226–#231 retain post-construction validation, target snapshots, and command detachment. #232/#233 keep Assignment allocation and OpenAPI aligned to the strictly-positive
(0, 1.0000]invariant. #246 preserves same-key/same-semantic replay of first-committed generic mutation identity. Packaging/runtime repairs align People withorgmetra-hris-kernel==0.4.0and Python>=3.12; #260/#261 own runtime-floor and installed-wheel evidence. #262 covers all four #64-owned operational UUID boundaries. #264 binds validated PostgreSQL connection capabilities structurally. #266–#269 detach command/result outer and nested identity aliases. #270 binds replay verification to an inert pre-port digest rather than mutable command/authorization objects.41a35a5b026d83cefae8d2ca6b2415bf36086d6ais the replay-verification repair head; Foundation34086056525later reached terminal SUCCESS and remains historical evidence only.Invalidated #271 experiment
Fresh causal review showed valid same-Employment writers already serialize through the current converted-worker row, while same-Position writers serialize through the Position root. #271's extra Employment row lock duplicated that conflict boundary without behavioral proof. Ordinary successor
19c6c7e2eb8c05d8317635a0fa66768cc904088bremoved the redundant lock and source-shape regression with no net file delta versus pre-#27141a35a5...; #271 is closednot_planned. No force-push or destructive rebase was used.PostgreSQL concurrency evidence — #272
fb28b01915cd8bf233260cfa3c4b562879f9ff7badded executable two-session PostgreSQL acceptance inservices/people-api/tests/test_postgres_assignment_concurrency_acceptance.py. It drives the realPostgresPeopleMutationPortunderREAD COMMITTEDagainst an isolated PostgreSQL 16 container and repository migrations. PostgreSQL itself must expose writer B inwait_event_type='Lock'with writer A inpg_blocking_pids(...); sleeps only pace observation.The two cases are same Employment/Person with different Positions, and different Employments/People with one Position. In each case writer A commits 0.7500 and writer B attempts 0.5000; B must re-read after the conflict boundary and reject an aggregate above
1.0000. Acceptance checks one durable Assignment, matching Assignment audit/outbox/idempotency evidence, no rejected-write residue, and client/server cleanup.015ba7ce98da5a3100194ec9e3389df8b0cedb5erepaired review findings in the harness (BaseException→Exception, unusedpytestremoval).1ea5cb1dcc887cf5b83fa85f926228e146f09d3fcorrected evidence terminology: seeded People/Employment/Position/Candidate/Decision records are deterministic synthetic integration fixtures, suitable for transaction/lock mechanism evidence only and not buyer-realistic or scientific claims. Foundation34107536416for exact1ea5cb1d...subsequently reached terminal SUCCESS.6cf31e8344a39d4490641441c190984533c49360repaired failure-path barrier release and started-writer joins.558afdf482af7e6e69ea0694e9f731365f1300beadded a forced post-lock failure regression proving server sessions disappear before teardown;07c2ff1107f9e39fcb74ec844a5972ca0c7d190aadded client-handle closure evidence. Test-firstfb8eeb9041c0ab838f1c03f0bdfb28500b4397ffshowed cleanup joins were unbounded, and614ee5f2027153223a6cb23773f2b8704674ca2dappliedjoin(timeout=30).c1fb77bdd87329755008df94b5161df205a96467forced deadline expiry after a real PostgreSQL lock observation.0d78299f5d49238fc1228f87b138d2bfca6dd367added_join_writer_before_teardown(...), which terminates an expired writer only by its captured PID plus internalapplication_nameand then bounded-joins again.b71263918cc59b9ca4177abf1770f1c87b619440strengthened the regression so the real termination query completes before the held client cleanup seam is released.Fresh review of
b712639...found that termination receipt itself was inconsistent. Ordinary forward52257a66f68cf0893f2b351cbc0392b3e52f5bb4changed the query to returncount(*)|coalesce(bool_and(pg_terminate_backend(pid)), false)::textand accept only exact1|true, so zero PID/application-name matches and failed termination both fail closed.Foundation
34127782377then supplied a real hosted RED on exact52257a66.... Exact checkout, compile, runner-image contract, foundation-pack validation, dependency hygiene, and all earlier owned suites passed; People ran 264 passing tests beforetest_expired_cleanup_join_terminates_live_backend_before_returningfailed. RCA: the regression seam forced the first bounded join to expire for both writer A and writer B, even though only writer B's client cleanup was deliberately held. After the pre-COMMIT barrier was released, writer A was allowed to finish naturally; it could quiesce between the synthetic zero-time join and PID/application-name lookup, so the exact termination receipt correctly returned no live owned backend. This was a test-seam defect, not evidence of a production row-lock or backend-termination defect.Ordinary forward
818950960c4fe0a4e7b026bd5499fe9cca1002beis the minimal causal repair._ExpireHeldWriterJoinThreadderives the targeted writer from the factory passed to_run_writer; only the deliberately held writer (factory._barrier is None) receives a synthetic first-join expiry. Writer A receives the real bounded cleanup join and may quiesce normally; writer B remains deliberately live so the real PID + internalapplication_nametermination query and exact1|truereceipt are still exercised. Onlytest_postgres_assignment_concurrency_failure_cleanup.pychanged. Production adapter code, SQL mutation semantics, transaction semantics, row-lock ordering, RLS, audit/outbox, and idempotency are unchanged.The real/right-cleared commercial/scientific acceptance tier remains separate and unresolved until a permitted provenance-backed source is available. Synthetic rows are not promoted to reality evidence.
Current exact-head acceptance
Foundation
34131641452, job101772911159, is terminal SUCCESS on exact818950960c4fe0a4e7b026bd5499fe9cca1002be. It proved exact checkout, compile, runner-image contract, foundation-pack validation, dependency hygiene, the complete owned unit/service invocation, repository PostgreSQL contracts, and read-only validation. The People API invocation runs the fullservices/people-api/teststree under the pinnedORGMETRA_POSTGRES_IMAGE, so #272's normal two-session interleavings plus forced post-lock failure/timeout cleanup are current-head hosted GREEN mechanism evidence.SAST
34131642172is terminal SUCCESS. Security34131642055is terminal FAILURE only atdependency-reviewsupport establishment; exact-head checkout succeeded and Scorecard, Trivy, and OSV completed successfully. The authoritative public dependency comparison remains the central fail-closed availability incident inContextualWisdomLab/.github#810; sibling scanner success is not promoted as a substitute.CodeQL
34131641597is terminal FAILURE. Language detection succeeded. Both Python and Actions compatibility jobs successfully read the current-head dispatch verdict and then failed atRelease runner or enforce current-head CodeQL verdictat2026-09-07T15:29Z; the same run'sDispatch current-head CodeQL scanjob did not start until16:01Zand then succeeded. This historical exact-head result remains non-passing and is a canary for the central terminal-receipt ordering/publication ownerContextualWisdomLab/.github#1929; later central repairs do not retroactively transfer GREEN evidence.Required Strix run
34131638369, job101787200568, is also terminal FAILURE. Its exact-head artifactstrix-reportsid10030120846has digestsha256:e61c95771ab6c37f7a60431f01eff4ada91adbcfbd2f6eaf79a803aacd7f9d5c. The scanner itself recordedstatus=completed,scan_completed=true, andsuccess=true, and emitted an empty SARIF result set after 191 LLM requests, but the terminal narrative simultaneously claimed several low-impact potential issues while provider-error traces remained in the report. The central strict gate therefore failed closed and labelled the runSTRIX_PROVIDER_UNAVAILABLE. That artifact is not promoted to GREEN: terminal evidence is internally inconsistent, and the provider-exhaustion diagnostic is not by itself proof that no usable route existed after the later completed scan receipt. Central owner issueContextualWisdomLab/.github#2026now owns the typed distinction among complete-consistent, incomplete/provider-unavailable, and complete-but-inconsistent Strix evidence while preservingorchestrator/free, no external/paid fallback, and default-null model timeout semantics.Submitted reviews remain COMMENTED-only; every returned inline review thread is resolved and qualifying independent approval is absent. Keep Draft; no merge/release claim is authorized.
Central Dependency Review availability, CodeQL terminal receipt publication, Strix terminal-evidence classification, and solo-maintainer approval-governance repairs remain
.githubowner-path concerns (#810,#1929,#2026,#772). Do not churn People source, fabricate verdicts, self/model approve, weaken required gates, transfer predecessor evidence, or use routine administrator bypass.Owner dependencies
#63 owns shared HRIS-kernel audit/runtime evidence. #55 owns People read semantics. #65 owns purpose-bound authorization/runtime integrity and overlaps People mutation files; after #64 becomes protected truth, #65 must non-force adopt it while preserving #229–#233, #246, packaging/runtime repairs, #262, #264, #266–#270, and #272's measured PostgreSQL behavior, evidence classification, deterministic failure cleanup, bounded waits, verified owned-backend termination, exact termination-receipt contract, and the rule that forced timeout evidence targets only a writer intentionally kept live. #141 follows #64/#65; #163/#165 consume normally integrated protected truth only.
docs/product-technical-gap-baseline.mdremains single-writer-owned by #100. Durable concurrency evidence should describe measured conflict serialization, deterministic cleanup, bounded failure waits, verified owned-backend termination, exact one-owned-session termination receipts, and targeted timeout seams; it must not resurrect #271's redundant lock or mislabel synthetic fixtures as real/right-cleared data.Causal order remains #63 protected integration as gates allow → #64 remaining exact-head gates/governance → #65 non-force protected adoption/reconciliation → #141 → Assignment descendants. No force-push, destructive rebase, self-approval, routine administrator bypass, gate weakening, no-op retrigger, mutable sibling source copy, or predecessor-evidence transfer.